-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sure all assertion rules run at least once #1586
Conversation
…lobs, imrpoved time zone handling, added logging, more accurately check if blob was moved
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
rs-e2e/src/main/java/gov/hhs/cdc/trustedintermediary/rse2e/ruleengine/AssertionRuleEngine.java
Show resolved
Hide resolved
PR Code Suggestions ✨Explore these optional code suggestions:
|
…all-assertions-run
…all-assertions-run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment on naming clarity and a request for a comment, otherwise looks good
def runRules = engine.runRules(outputMessage, inputMessage) | ||
toRunRules.removeAll(runRules) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the naming here might be confusing to come back to in the future since the run
in runRules
can either be a verb (like 'go run the rules') or an adjective ('run rules are rules that already ran'). Maybe they could be evaluatedRules
and rulesToEvaluate
, or rulesThatRan
and rulesToRun
, or some other pair that's slightly more distinct from the verb-based method names?
It might also be worth adding a small comment here, like //Check whether all the rules in the assertions file have been run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll change the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! No notes.
Quality Gate passedIssues Measures |
Description
Validate that all assertion rules in
assertion_definitions.json
are used at least once int the Automated TestIssue
#1525